document-portal: use default FUSE implementation for statfs #1807
Conversation
| .link = xdp_fuse_link, | ||
| .flush = xdp_fuse_flush, | ||
| .statfs = xdp_fuse_statfs, | ||
| /* .statfs = xdp_fuse_statfs, */ |
There was a problem hiding this comment.
What information does the default implementation provide?
If fuse's default implementation does everything we need, then please delete this instead of commenting it out, and delete the now-unused implementation of xdp_fuse_statfs(). There's no point in keeping dead code in-tree, it's just a source of compiler warnings: we can always get it back from the git history if we need it.
There was a problem hiding this comment.
But... would the default implementation allow a sandboxed app to distinguish between the existence and nonexistence of a document that it does not have access to? If it would, then using the default implementation would be an information disclosure vulnerability.
What is broken about it is a more interesting fact. Looking at that implementation, it seems to be assuming that it's being called for an inode that represents a document, for which the safe fallback is to fail with In some of the other interfaces like Probably the right logic for |
|
The default implementation of fuse uses |
smcv
left a comment
smcv
left a comment
There was a problem hiding this comment.
The default implementation of fuse uses memset too to create the dummy data for statfs
I think you might be reading the wrong part of libfuse: do_statfs() in lib/fuse_lowlevel.c looks like it's the default implementation that would be used if we didn't supply one, and it fills in f_namemax = 255 and f_bsize = 512. I assume there must be some compatibility reason why it needs to fill those; if yes, we should probably do the same.
I'd be inclined to initialize buf like libfuse does, rather than memset'ing it and then filling in certain members - that lets the compiler choose whatever is the most optimal way to achieve the desired result.
| if(xdp_domain_is_virtual_type(inode->domain)) { | ||
| memset(&buf, 0, sizeof(buf)); | ||
| fuse_reply_statfs (req, &buf); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Please indent consistently with the surrounding code:
| if(xdp_domain_is_virtual_type(inode->domain)) { | |
| memset(&buf, 0, sizeof(buf)); | |
| fuse_reply_statfs (req, &buf); | |
| return; | |
| } | |
| if (xdp_domain_is_virtual_type (inode->domain)) | |
| { | |
| memset (&buf, 0, sizeof (buf)); | |
| fuse_reply_statfs (req, &buf); | |
| return; | |
| } |
| const char *op = "STATFS"; | ||
|
|
||
| g_debug ("STATFS %" G_GINT64_MODIFIER "x", ino); | ||
|
|
There was a problem hiding this comment.
I think this deserves a comment as to why this is appropriate, perhaps something like:
| /* Free space, etc. don't make much sense for a virtual filesystem, | |
| * so reply with a mostly empty buffer. This is consistent with what libfuse does | |
| * if we don't supply a statfs implementation. */ |
There was a problem hiding this comment.
(but obviously that comment is misleading if what we do here is not consistent with what libfuse would have done without a statfs implementation!)
|
Hey @SlonkeyMaster, are you going to pick this PR up again? |
3 participants
For a little shy of 5 years, the
xdg-document-portalimplementation of thestatfssyscall has been broken and returned EPERM. The default FUSE implementation returns a sensible default value, similar to the/procand/sysmount points. This merge request comments out the broken implementation in favor of FUSE's default, and fixes #553.I understand this is not an ideal implementation of this FUSE interface. On the other hand, this change breaks no test, and allows
df -a(and any other program using the statfs syscall) to run in harmony withxdg-document-portal.